fix: use Bun's implementation of ws instead of the bundled one
#13901
Conversation
packages/vite/src/node/config.ts
Outdated
| // Bun will cache file/directories, and since we just created the file it might throw a ResolveError | ||
| // Adding a query string with the `?` will bust the cache. | ||
| // See https://github.com/oven-sh/bun/issues/3216 | ||
| const fileUrl = `${pathToFileURL(fileBase)}.mjs?` |
There was a problem hiding this comment.
Not oppose to this change, but I feel like Bun should ultimately fix this too. Currently the issue is closed and appending ? is the way in Bun?
There was a problem hiding this comment.
Agreed, we could move forward with this one but it will be good to remove this hack as soon as Bun works out this bug. I think we should only append ? when process.versions.bun.
There was a problem hiding this comment.
Bun's builtin TypeScript, JSX, ESM, and CommonJS support means transpiling a configuration file ahead of time should be unnecessary. Reading, transpiling, saving and then reading the configuration file again makes Vite start slower than it could. Rather than conditionally adding a ?, what if we plan to disable this step entirely in Bun?
There was a problem hiding this comment.
That would lose slight parity with the current config loader, as it tracks the dependencies of the config to trigger a server restart. Personally, I think this "feature" has blocked us from simplifying a lot of things that I'd rather not have it (around Vite 5? Not sure yet).
So I guess we have two options (that I'm fine either ways):
- Use
?cache bust, but Bun should fix this eventually. - Load config with Bun without deps restart trigger. Bun users have slightly different experience.
There was a problem hiding this comment.
I lean toward using ? here if we can't get the same experience.
There was a problem hiding this comment.
@Jarred-Sumner replying to your comment (#13901 (comment)), I think it would be best if we can implement the least changes on our end here to support Bun. If no2 requires using more Bun-specific APIs, that would likely create another code path to maintain.
Is oven-sh/bun#3216 (the ? issue) not fixable on Bun's end? I feel like this quirk could affect more libraries in general than only Vite.
There was a problem hiding this comment.
The reason we put the ? is because the fix for bun isn't trivial. I agree we should fix this in bun, but it might not happen today for 0.7.
|
@paperdave thanks for the PR. Would you separate the directory caching bug part on a separate PR so we can better track both changes? (and revert this one later if we go forward with it). We would like to know for this part what are your plans, as it seems we are patching a bug that may be solved soon in Bun. This PR could remain about Bun's own version of |
|
Load config with Bun without deps restart trigger. Bun users have slightly different experience.
In Bun, you can do Loader.getDependencyKeysIfEvaluated and it will call
this function which returns an array of module specifiers a module depends
on
https://github.com/WebKit/WebKit/blob/2f688c542e6be822158997052c6fe6f6f63c4c16/Source/JavaScriptCore/builtins/ModuleLoader.js#L593
I don’t feel good we expose this JSC internal api so directly because they
may change it randomly later, but we can expose a wrapper for this
Another thing in Bun is that require.cache can be used to invalidate ESM
modules. require.cache is implemented as a Proxy which merges the real
CommonJS module cache and the ESM registry cache into one
On Thu, Jul 20, 2023 at 6:02 AM Bjorn Lu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/vite/src/node/config.ts
<#13901 (comment)>:
> + // Bun will cache file/directories, and since we just created the file it might throw a ResolveError
+ // Adding a query string with the `?` will bust the cache.
+ // See oven-sh/bun#3216
+ const fileUrl = `${pathToFileURL(fileBase)}.mjs?`
That would lose slight parity with the current config loader, as it tracks
the dependencies of the config to trigger a server restart. Personally, I
think this "feature" has blocked us from simplifying a lot of things that
I'd rather not have it (around Vite 5? Not sure yet).
So I guess we have two options:
1. Use ? cache bust, but Bun should fix this eventually.
2. Load config with Bun without deps restart trigger. Bun users have
slightly different experience.
—
Reply to this email directly, view it on GitHub
<#13901 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFNGS4MW66HCNIZNBTEYVLXRET5NANCNFSM6AAAAAA2QYJQQQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
(Meant as a reply to a review comment but shows up as a top level comment because email )
|
I think that is reasonable. I'll split this into two PRs. |
paperdave
changed the title
ws instead of the bundled one
|
the |
|
Thanks for moving the |
|
legendary. were gonna release bun 0.7 tomorrow (i think) so that will be when users can test it out. |
Description
There are two small issues that are blocking Vite from working in Bun, which is much easier to fix in the Vite project itself.
Bun implements it's own version of
ws, which gets overridden when doingimport {...} from "ws". Vite bundles it's own copy of ws which attempts to usereq.socket.writewhich we do not implement as we do not use a node stream for the underlying network sockets. Implementing the APIs needed for the JS implementation ofwswould take too long and be much slower.During config loading there is a directory caching bug, the workaround we are using is applying a query string parameter to the
file://url used when loading the config through esm.That is all that it takes to get Vite working on Bun, though it depends on a handful of various fixes we recently made to our node compatibility. To test these changes, you need to use the canary version
bun upgrade --canary(or 0.7 when released), and then usebun --bun run dev(--bun forces bun's runtime over node's).Closes oven-sh/bun#250
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).